-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the design document #108
Conversation
156f18a
to
7eb196d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to document all this valuable information! Left some feedback comments (happy to push changes directly if you're ok too). In addition, I think we should delete this section from the readme since it is expanded upon here.
|
||
As of 2024-02-09, the user is expected to launch the router by hand. | ||
In the future, additional work may be done to automatically launch a Zenoh router if one isn't already running. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mentioned a couple lines about the default router config and how it is configured to disable multicast scouting and enables gossip scouting instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up, should we include a section on the session config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go ahead and add a section for both of these.
While looking at this, I realized that our environment variables are named ZENOH_ROUTER_CONFIG_URI
and ZENOH_SESSION_CONFIG_URI
. But we don't treat them as URIs, we treat them as filenames. Should we change that somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Co-authored-by: Yadu <[email protected]> Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
95ee937
to
051c630
Compare
@Yadunund I've responded to most of your feedback now. There are a couple of pieces of information that I don't have, so I didn't update those. If you can take another look, that would be appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome!
This PR does a large rewrite of the design document, adding in a lot more detail about how rmw_zenoh_cpp conforms to the RMW interface.
I'll note that this document isn't complete; in particular, the section on how Events works is incomplete (until we get #103 in), and the section on Security is completely empty. Nevertheless, I think it expand quite a bit on what we currently have and will give users some idea on the choices we made and why.